Contact Form: Revised the email validation regex to include length limit #9323

stevenlinx · 2018-04-16T16:24:01Z

Changes proposed in this Pull Request:

Revised the email validation regex to include length limit, in order to match the latest practical implementation of RFC 5322 in regular-expressions.info/email.html.
Regex used:

PHP version: the very bottom one on regular-expressions.info/email.html. The one after the sentence "...modern regex flavors aren't truly regular, so we can add length limit checks using lookahead like we did before:" Take note, the redundant pink-colored white spaces in the middle of the regex must be removed.

\A(?=[a-z0-9@.!#$%&'*+/=?^_`{|}~-]{6,254}\z)(?=[a-z0-9.!#$%&'*+/=?^_`{|}~-]{1,64}@)[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:(?=[a-z0-9-]{1,63}\.)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+(?=[a-z0-9-]{1,63}\z)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\z

JavaScript version:

^(?=[a-z0-9@.!#$%&'*+/=?^_\`{|}~-]{6,254}$)(?=[a-z0-9.!#$%&'*+/=?^_\`{|}~-]{1,64}@)[a-z0-9!#$%&'*+/=?^_\`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_\`{|}~-]+)*@(?:(?=[a-z0-9-]{1,63}\.)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+(?=[a-z0-9-]{1,63}$)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$

Testing instructions:

1.) Checkout the code in this PR on a test WP site.
2.) Use the Contact Form and enter some test data in each field.
3.) In the email field, test different email addresses that are expected to pass and fail.
4.) Use an email you've access to, submit the form and make sure you get the email.
5.) Revert the code back to its previous state.

Proposed changelog entry for your changes:

Contact Form: Revised the email validation regex to include length limit.

oskosk

LGTM! Thank you!

dereksmart

This is not validating properly.

It's even checking for literal spaces right now: https://regex101.com/r/MAOFqc/4

var newRe = /(?=[a-z0-9@.!#$%&'*+/=?^_`{|}~-]{6,254}\z)  (?=[a-z0-9.!#$%&'*+/=?^_`{|}~-]{1,64}@)  [a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)* @ (?:(?=[a-z0-9-]{1,63}\.)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+   (?=[a-z0-9-]{1,63}\z)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?/i;

var oldRe = /[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?/i;

newRe.test( 'email@gmail.com' );
false
oldRe.test( 'email@gmail.com' );
true

When I tested, I made a mistake and tested with the minified version (which doesn't include the changes proposed here)

stevenlinx · 2018-04-26T15:29:28Z

I think I got what was causing the issues. The regex was correct but two things must be done to apply it to javascript, due to regex flavors:

1.) The \A and \z must be either be replaced (with caret ^ and dollar $) or removed (if the subject string is the email string) since javascript does not support \A and \z, but only ^ and $.

For the previous regex on the website, there were only \A and \z at the beginning and end.

Unlike the previous regex, the latest regex version on the website (the very bottom one) not only has \A and \z in the beginning and end, but also \z in the middle (due to lookahead). In my previous commit, I only removed \A and \z in the beginning and end, but not all the \z the middle.

2.) The pink-colored white spaces in the middle of regex must also be removed, which was part of web page layout. In the previous regex, there were no white space that need to be removed.

I've push a new commit with the revised regex.

=====

Tests:

user@example.com
https://regex101.com/r/sGpumM/1

local part (64 char.):
https://regex101.com/r/AZTNg9/1
expected: pass
result: pass

local part (65 char.):
https://regex101.com/r/AZTNg9/2
expected: fail
result: fail

part of domain (63 char.):
https://regex101.com/r/AZTNg9/3
expected: pass
result: pass

part of domain (64 char.):
https://regex101.com/r/AZTNg9/4
expected: fail
result: fail

total length (254 char.):
https://regex101.com/r/AZTNg9/5
expected: pass
result: pass

https://regex101.com/r/AZTNg9/6
expected: fail
result: fail

oskosk · 2018-08-23T19:53:38Z

@stevenlin-x can you please clarify in this PR's description which expression is the one you're taking as appropriate from https://www.regular-expressions.info/email.html and then, right next to it, the JS version you're implementing.

Some time has passed and there's a lot of regex translation happening

stevenlinx · 2018-09-02T08:00:09Z

I've added the regex of both PHP and JS versions to the PR description.

Feedback was addressed

brbrr · 2019-01-22T06:12:25Z

I played a bit with this PR. I made a simple jsFiddle with some valid and invalid emails according to Wiki and this post

https://jsfiddle.net/f9dkotb8/1/

Here are the results:

====================
Testing valid emails
====================
email@example.com isValid: true
firstname.lastname@example.com isValid: true
email@subdomain.example.com isValid: true
firstname+lastname@example.com isValid: true
email@123.123.123.123 isValid: true
email@[123.123.123.123] isValid: **false**
"email"@example.com isValid: false
1234567890@example.com isValid: true
email@example-one.com isValid: true
_______@example.com isValid: true
email@example.name isValid: true
email@example.museum isValid: true
email@example.co.jp isValid: true
firstname-lastname@example.com isValid: true
much."more unusual"@example.com isValid: **false**
very.unusual."@".unusual.com@example.com isValid: **false**
very."(),:;<>[]".VERY."very@\\\ "very".unusual@strange.example.com isValid: **false**

====================
Testing invalid emails
====================
plainaddress isValid: false
#@%^%#$@#$@#.com isValid: false
@example.com isValid: false
Joe Smith <email@example.com> isValid: false
email.example.com isValid: false
email@example@example.com isValid: false
.email@example.com isValid: false
email.@example.com isValid: false
email..email@example.com isValid: false
あいうえお@example.com isValid: false
email@example.com (Joe Smith) isValid: false
email@example isValid: false
email@-example.com isValid: false
email@example.web isValid: **true**
email@111.222.333.44444 isValid: **true**
email@example..com isValid: false
Abc..123@example.com isValid: false
"(),:;<>[]@example.com isValid: false
just"not"right@example.com isValid: false
this is"really"not\\allowed@example.com isValid: false

@stevenlin-x do you think we should cover these failing cases too?

brbrr · 2019-01-28T09:10:45Z

FWIW: I looked into ways to cover this change with unit-tests. Unfortunately, it's not easy(or even possible) to write tests for a specific function for this file due to its structure. We might want to refactor it to make sure every function is testable.

stevenlinx · 2019-03-03T15:04:02Z

@brbrr

1.)
In the code samples you provided above, if you replace the regex with the original regex (i.e. the present regex in use in production), it will also result in false positives with those valid and invalid emails samples.

In other words, you didn't read the context of the ticket in full.

2.)
You falsely thought this was a literal RFC 5322 validation exercise when it's not.

The present regex in use was derived from
regular-expressions.info/email.html

Issue #3734 pointed out the regex no longer matches the one on the updated page.

So I simply updated to reflect the character length limits.

3.)
If you think those samples should be addressed, you should ask the author of the regular-expressions.info site or you should open a separate ticket and provide the reasons on why those samples should be addressed.

Or even better, provide the regex you would use.

stale · 2019-06-02T12:52:02Z

This PR has been marked as stale. This happened because:

It has been inactive in the past 3 months.
It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

matticbot · 2019-10-08T15:07:48Z

Caution: This PR has changes that must be merged to WordPress.com
Hello stevenlin-x! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33713-code before merging this PR. Thank you!

kraftbj

Apologies for the massive delay on this one. While there are other regex things to improve to ensure that all valid e-mails are accepted and all invalid are excluded, this is a step improvement and we shouldn't let perfect be the enemy of good.

Approving and open to future PRs to address the other issues (or investigate using browser-based HTML5 validation instead of the JS solution).

matticbot · 2019-10-08T15:15:55Z

stevenlin-x, Your synced wpcom patch D33713-code has been updated.

* 7.9: Changelog * Update version number * Update stable tag and tested up to * Changelog: add #13530 * changelog: add #13578 * Changelog: add #13598 * Changelog: add entry for numerous block preview changes * Changelog: add #13599 * changelog: add #13541 * Changelog: add #13542 * Changelog: add #13331 * Changelog: add #13558 * Changelog: add #13409 * Changelog: add #13582 * Changelog: add #13600 * Changelog: add #13601 * Changelog: add #13595 * Changelog: add #12695 * Changelog: add #13009 * Changelog: add #13649 * Changelog: add #13450 * Changelog: add #13507 * Changelog: add #13658 * Changelog: add #13687 * changelog: add #13683 * Changelog: add #9323 * Changelog: add #13681 * Fix typos in readme * Add link to WordPress Beta Tester plugin * Changelog: add #13630 * Changelog: add #13695 * Changelog: add #13659 * Changelog: add #13716 * Changelog: add #13664 * Changelog: add #13682 * Changelog: add #13362 * Changelog: add #13563 * Add testing list for #13563 * Changelog: add #13735 * Changelog: add #13752 * Changelog: add #13624 * Changelog: add #13756 * Changelog: add #13745 * Changelog: add #13728 * Changelog: add #13779 * Changelog: add #13699 * Changelog: add #13804 * Changelog: add #13761 * Changelog: add #13637 * Changelog: add #13517 * Changelog: add #13521 * Changelog: add #13729 * Testing list: add testing instructions for #13729 * Changelog: add sync changes * Changelog: add #13807 * Changelog: add #13654 * Changelog: add #13795 * Changelog: add #13801 * Changelog: add #13818 * Changelog: add #13725 * Changelog: add #13831 * Changelog: add #13516 * Testing list: add Twenty Twenty instructions * Changelog: add #13799 * Changelog: add #13805 * Changelog: add #13688 * Changelog: add #13830

revise the email validation regex to add length limit

36169e8

stevenlinx requested a review from georgestephanis as a code owner April 16, 2018 16:24

oskosk added this to the 6.1 milestone Apr 17, 2018

jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Contact Form [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 17, 2018

stevenlinx changed the title ~~Contact Form: Revise the email validation regex to add length limit~~ Contact Form: Revised the email validation regex to add length limit Apr 17, 2018

stevenlinx changed the title ~~Contact Form: Revised the email validation regex to add length limit~~ Contact Form: Revised the email validation regex to include length limit Apr 17, 2018

oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Apr 24, 2018

oskosk previously approved these changes Apr 24, 2018

View reviewed changes

dereksmart previously requested changes Apr 24, 2018

View reviewed changes

dereksmart added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 24, 2018

dereksmart removed this from the 6.1 milestone Apr 24, 2018

revised regex again: removed white space and replaced \z in the middle

82ee7f2

dereksmart added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jun 20, 2018

brbrr added this to the 6.5 milestone Aug 21, 2018

oskosk added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Aug 23, 2018

abidhahmed modified the milestones: 6.5, 6.6 Aug 28, 2018

jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 5, 2018

This comment has been minimized.

Sign in to view

stale bot added the [Status] Stale label Dec 10, 2018

matticbot added the Touches WP.com Files label Dec 10, 2018

stale bot removed the [Status] Stale label Dec 10, 2018

brbrr added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 22, 2019

dereksmart removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Feb 11, 2019

jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 4, 2019

stale bot added the [Status] Stale label Jun 2, 2019

stale bot removed the [Status] Stale label Oct 8, 2019

kraftbj added this to the 7.9 milestone Oct 8, 2019

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 8, 2019

kraftbj approved these changes Oct 8, 2019

View reviewed changes

kraftbj added the [Status] Tested on WP.com label Oct 8, 2019

jeherve merged commit 65bd4db into Automattic:master Oct 9, 2019

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 9, 2019

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Oct 17, 2019

jeherve added a commit that referenced this pull request Oct 23, 2019

Changelog: add #9323

f8cc8bc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contact Form: Revised the email validation regex to include length limit #9323

Contact Form: Revised the email validation regex to include length limit #9323

stevenlinx commented Apr 16, 2018 •

edited

Loading

oskosk left a comment

dereksmart left a comment •

edited

Loading

stevenlinx commented Apr 26, 2018 •

edited

Loading

oskosk commented Aug 23, 2018

stevenlinx commented Sep 2, 2018 •

edited

Loading

This comment has been minimized.

brbrr commented Jan 22, 2019 •

edited

Loading

brbrr commented Jan 28, 2019

stevenlinx commented Mar 3, 2019 •

edited

Loading

stale bot commented Jun 2, 2019

matticbot commented Oct 8, 2019

kraftbj left a comment

matticbot commented Oct 8, 2019

Contact Form: Revised the email validation regex to include length limit #9323

Contact Form: Revised the email validation regex to include length limit #9323

Conversation

stevenlinx commented Apr 16, 2018 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

oskosk left a comment

Choose a reason for hiding this comment

dereksmart left a comment • edited Loading

Choose a reason for hiding this comment

stevenlinx commented Apr 26, 2018 • edited Loading

oskosk commented Aug 23, 2018

stevenlinx commented Sep 2, 2018 • edited Loading

This comment has been minimized.

brbrr commented Jan 22, 2019 • edited Loading

brbrr commented Jan 28, 2019

stevenlinx commented Mar 3, 2019 • edited Loading

stale bot commented Jun 2, 2019

matticbot commented Oct 8, 2019

kraftbj left a comment

Choose a reason for hiding this comment

matticbot commented Oct 8, 2019

stevenlinx commented Apr 16, 2018 •

edited

Loading

dereksmart left a comment •

edited

Loading

stevenlinx commented Apr 26, 2018 •

edited

Loading

stevenlinx commented Sep 2, 2018 •

edited

Loading

brbrr commented Jan 22, 2019 •

edited

Loading

stevenlinx commented Mar 3, 2019 •

edited

Loading